-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: change testAuth*
strategy
#374
Conversation
Outsider comment, curious about your opinion, totally agree that these tests are making the testing suite last A LOT more and unresponsive until they're finally checked (~3m in my computer), also creating a bunch of requests to the RPC that could trigger a DoS when using a free service. I wonder if we could think of a more holistic approach, that doesn't involve adding even one more check to both crafter and reviewer tasks, considering that, as long as these tasks keep growing, we can expect to each of them be "less important" to check, and perhaps to be overlooked in some PR. I see the handling of Questions on the underlying rationale of these tests (again, have nothing to do with this, so feel free to answer or ignore):
And the more holistic approach comment, perhaps to be implemented with another workflow i'm thinking about (gonna open an issue about):
contract ChainlogChanges {
updates.push('MCD_FLOP', <from>, <to>)
newContracts.push('CONTRACT', <address>)
} |
Submitted a PoC PR in #375 |
Every wallet that deploys contracts to be used in Maker Protocol MUST be added to the There's no easy way to perform an on-chain check that no other address is present in The process is designed to work with this whitelist because any address outside of it SHOULD NOT be allowed. Of course, since the list is manually kept, there's room for human error, which we try to mitigate by having a comprehensive checklist that must be strictly followed and a minimum requirement of 2 reviewers to be able to ship the code.
This check is only relevant for contracts that have special permissions within the Maker Protocol. A contract with such would never get past internal reviews or external audits. External contracts never have any permissions in the system, so even in the unlikely scenario a contract behaves like that (it would be suspicious to say the minimum), there would be no effect, so the test output can be ignored. Regarding your suggestions, I'll answer in the PR you opened. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the danger of this design is that it increases the trust assumptions on the correct completion of the added steps of makerdao/pe-checklists#17 and the effect would be "a test not running", which is very easy to miss on a code review, and reduces the overall testing security (in a critical vector) for the tradeoff of making it resource efficient
Notice that this is not the only check performed. On L110 there's a manual check for dangling permissions. Regarding the effect of a "test not running", I believe we can leverage the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a major change to the way wards are checked right now, although I understand that it's unlikely that we need to re-check every chainlog address in every spell. I also certainly like that the PR brings almost instant test runs. But since this change would nevertheless decrease the amount of checks in the base tests, I want to ask if you considered using multicall pattern to potentially speed up the tests without modifying their logic and the spell process?
If we decide to stick with "not checking every chainlog contract every time", I think we should:
- Add new
_checkWardsOrSrcWards
that would automatically check if wards are present in the contract or in the source and check them, instead of requiring developer to pick the right function - Run
_checkWardsOrSrcWards
inside_checkChainlogKey
to always check for wards in all newly added contracts, instead of requiring developer to add a separate test
Alternatively or additionally to the above, we can also run _checkWardsOrSrcWards
on all chainlog addresses as soon as there were calls to the chainlog from the spell (e.g. detecting it via vm.expectCall
)
I am not aware that we can use multicall within Foundry directly. We could potentially write a proxy server that would collect the requests, build a multicall and then answer those, but that seems like a huge effort in terms of maintenance.
I don't oppose this idea directly, however I don't believe there is a lot to gain here. One of the reasons why I renamed the test from OSMs are being deprecated, the next generation of oracles will not feat them. Also, the latest I believe it's very unlikely that we will onboard new OSMs to the protocol at this point, which means this specific test could potentially be removed in the future.
That's not a bad idea. The only drawback is that the check is not explicit anymore, and the test would serve multiple purposes. We could potentially move the auth tests closer to
Please correct me if I'm wrong, but |
Multicall is just a contract that can batch generic read requests together. Maker even have a set of maker-developed and maker-deployed contracts here. I think that packing all
I don't agree that it's a drawback, since it specifically meant to prevent human error.
|
Oh, I thought you were referring to RPC multicall. I'll push some changes addressing part of your suggestions. |
Oh, ok. I think I got the idea! |
I believe this is a good practice when dealing with a JS + Solidity environment, we actually use it on a lot of frontends, deploy a contract that batches the requests into a single call and let the RPC handle the rest. function checkAuth(bytes32[] memory contractNames, address deployer) {...} In Foundry, tho, since tests are already a contract call that does that, the benefits of "batching" are not present, the processing would happen or in this "batch contract", or inside the test method, in the end, is the same. The problem with these tests are really the amount of data to be read, +8000 storage reads. I agree that the test wasn't also a line of defense vs unwanted authorizations happening, since it was only checking the deployer addresses, and currently like the current design, where these tests run when a contract is added to the Chainlog instead. 👍 |
Regarding the I'm trying locally with: function _executeSpell() external {
_vote(address(spell));
_scheduleWaitAndCast(address(spell));
assertTrue(spell.done());
}
function _checkSpellDoesNotModifyAddressesInChainlog() external {
vm.expectCall(address(chainLog), abi.encodeWithSelector(chainLog.setAddress.selector), 0);
this._executeSpell();
}
function _testFullAuth() internal {
uint256 initial = vm.snapshot();
try this._checkSpellDoesNotModifyAddressesInChainlog() {
console2.log("[testFullAuth] Chainlog not modified, skipping...");
return;
} catch {
console2.log("[testFullAuth] Chainlog modified, running...");
}
vm.revertTo(initial);
_vote(address(spell));
_scheduleWaitAndCast(address(spell));
assertTrue(spell.done());
bytes32[] memory keys = chainLog.list();
for(uint256 i = 0; i < keys.length; i++) {
console2.log("[testFullAuth] Checking key:", string(abi.encodePacked(keys[i])));
// _checkAuth(keys[i]); // skiping this for testing purposes
}
} If the spell violates
I even tried to put calls in |
Yes, there are even libraries which nicely do those calls for you. I also know this approach from the frontend world where it makes sense for multiply reasons (getting info on the same block, saving request quota, etc). But looking at your explanation I'm not yet convinced it wouldn't help. From the investigation we know that the main bottleneck is throttling of the requests that happening on the provider side. Multicall suppose to help with that.
I think you can use the address mentioned here or the newer multicall3 address.
Yes, correct. Having checks inside
Thanks for the code, I will also look into it 🙂 |
With There is tho, if willing to do an extra test, a way of calling |
} | ||
|
||
// Account for new keys | ||
// Notice: we don't care about removed keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in removed key not being covered by those two automatic tests (except indirectly, via version change). I think it's fine overall, especially in regards to wards
, but just wanted to emphasize it.
Was that the reason why you kept testRemoveChainlogValues
inside DssSpell.t.sol
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a clear pattern here AFAIK.
Should we scuttle any contract being removed from the chainlog?
There is an easy-ish way to figure out removed keys automatically and run the scuttle checks.
If we don't care about enforcing this, then maybe testRemoveChainlogValues
can be removed.
Since I was not sure about this, I didn't make any changes to this specific test, but I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put it out of scope of this refactoring. But adding a special test case to check that removed contracts doesn't have any wards is a good improvement idea
Without this check, there would be a case where changes to the chainlog not followed by a version bump would actually make the test pass. Imagine this: ``` KEY A -> ADDRESS A KEY B -> ADDRESS B KEY C -> ADDRESS C ``` If the spell removes `KEY C` and adds `ANOTHER KEY` with `ADDRESS C` as a value, we'd get: ``` KEY A -> ADDRESS A KEY B -> ADDRESS B ANOTHER KEY -> ADDRESS C ``` This should cause a version update, but the test is not enforcing it.
9c25724
for (uint256 i = 0; i < keys.length; i++) { | ||
assertEq( | ||
chainLog.getAddress(keys[i]), | ||
addr.addr(keys[i]), | ||
_concat("TestError/chainlog-vs-harness-key-mismatch: ", keys[i]) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering the PR objective was initially to reduce testing times, this check could be done inside _testChainlogIntegrity
, when creating the cacheAfter
object, optimizing 1/3 of the calls, since it'll have to query all chainlog values only twice (before spell and after spell)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foundry caches duplicated calls, so this won't result in new RPC calls being made.
ca39a08
Context
A couple of months ago, Dewiz wrote a blog post exploring the execution costs of the spell test suite.
During the analysis, we found out that the
testAuth
andtestAuthInSources
test cases alone correspond to the majority of the requests made against the RPC node.Those tests aim to prevent the deployer of contracts being added to the protocol from keeping
wards
privilege over them.The main issue is that they are structured as a "shotgun" solution: the tests run every time (even if there are no new contracts), iterate over the entire chainlog (which contains 400+ contracts on Mainnet) and over the deployers list on
addresses_deployers.sol
(20+ addresses) in a nested loop.That alone leads to 8000+ unique requests to the RPC provider, which ends up being heavily throttled, leading to very large execution times.
Proposal
To alleviate the problem, I propose we change
testAuth*
to a more incremental approach: contracts are only checked for danglingwards
when they are first added to the protocol.If the permissions are correctly set during the onboarding, the only way to modify the
wards
of a given contract is through another spell, however the current spell review process would not allow for that. In other words, there is no additional gain in terms of security bought by checking every single contract against every single deployer address on every single test run, we just need to ensure that the onboarding is properly done.To make sure this step won't be missed, there's an accompanying PR on makerdao/pe-checklists#17 to explicitly instruct crafters and reviewers to include such tests when they are required.